-
Notifications
You must be signed in to change notification settings - Fork 383
fix: Admin Client disconnect and closed when ban or mute a player #2783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where the Admin Client would disconnect and close unexpectedly when banning or muting a player. The root cause was improper disposal handling in the BanMuteBox dialog—calling Dispose() directly instead of using the Close() method, which led to premature resource cleanup and client disconnection.
Key Changes
- Modified the BanMuteBox to call
Close()instead ofDispose()on button clicks, allowing proper window lifecycle management - Refactored the Base.Dispose() method to handle re-entrant disposal more gracefully by returning early instead of throwing exceptions
- Improved data structure for duration options from Dictionary to List of tuples for better maintainability
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| Intersect.Client.Framework/Gwen/Control/Base.cs | Refactored Dispose() to silently return if already disposed instead of throwing ObjectDisposedException; added documentation comments; adjusted formatting for InputHandler cleanup; added DEBUG conditional for _disposeStack |
| Intersect.Client.Core/Interface/Game/Admin/BanMuteBox.cs | Changed button click handlers to call Close() instead of Dispose(); refactored duration options from Dictionary to List; added null safety checks; changed "Forever" duration from 999999 to int.MaxValue; added default duration selection logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (InputHandler.HoveredControl == this) | ||
| { | ||
| InputHandler.HoveredControl = null; | ||
| } | ||
|
|
||
| if (InputHandler.KeyboardFocus == this) | ||
| { | ||
| InputHandler.KeyboardFocus = null; | ||
| } | ||
|
|
||
| if (InputHandler.MouseFocus == this) | ||
| { | ||
| InputHandler.MouseFocus = null; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The braces were removed from these if statements, making the code more compact but potentially less maintainable. While this follows some coding styles, it can increase the risk of bugs if additional statements are later added without re-adding braces. Consider whether consistency with the project's style guide is maintained.
| { | ||
| private readonly TextBox _textboxReason; | ||
| private readonly ComboBox _comboboxDuration; | ||
| private readonly ComboBox? _comboboxDuration; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _comboboxDuration field is marked as nullable (ComboBox?), but it's initialized in the constructor on line 62 and should never be null during the object's lifetime. Making it nullable adds unnecessary null-checking overhead (lines 94, 119) without providing value. Consider making this field non-nullable to better reflect its actual behavior.
| catch | ||
| { | ||
| throw; | ||
| return; |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code used ObjectDisposedException.ThrowIf to throw an exception if the object was already disposed. The new code silently returns early instead. This changes the behavior from failing loudly (which helps catch double-disposal bugs during development) to failing silently. Consider whether this is the desired behavior change, as the original pattern helped identify misuse of disposed objects. If the goal is to prevent exceptions, document why silent failure is acceptable in this case.
| return; | |
| throw new ObjectDisposedException(GetType().FullName); |
|
|
||
| if (_textboxReason != null) | ||
| { | ||
| Interface.FocusComponents.Remove(_textboxReason); | ||
| } | ||
|
|
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null-check for _textboxReason is unnecessary. The field is initialized in the constructor (line 52) and is not nullable, so it will never be null at this point in the Dispose method. This defensive check adds complexity without providing value.
| if (_textboxReason != null) | |
| { | |
| Interface.FocusComponents.Remove(_textboxReason); | |
| } | |
| Interface.FocusComponents.Remove(_textboxReason); |
| okayHandler?.Invoke(this, EventArgs.Empty); | ||
| Dispose(); | ||
| Close(); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Okay button now calls Close() instead of Dispose(), but AdminWindow.cs still explicitly calls Dispose() on the _banOrMuteWindow after the okayHandler is executed (lines 481 and 512 in AdminWindow.cs). This creates a double-disposal scenario: Close() is called first (line 82), followed by an explicit Dispose() call. Even though the Dispose() method now checks for _disposed and returns early, this pattern is confusing and could lead to issues if DeleteOnClose is set to true on this window (which would cause automatic disposal on Close()). Consider removing the explicit Dispose() calls in AdminWindow.cs.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Palit <[email protected]>
|
This is not the fix for this issue. |
Fixes #2782